-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: fix readable event not emitted (fixes #25969) #48866
Conversation
Review requested:
|
fix readable event not emitted after reconnect Fixes: nodejs#25969
1bb06be
to
07b336d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a PR! Can you please add a unit test?
I'll add it later |
@mcollina I'm sorry, but I don't know how to test this, I have tested it locally, it works now.
please help me. client.js
const net = require('net');
const client = new net.Socket();
let connected = false;
client.on('error', (err) => console.error('client#error', err.message));
client.on('close', () =>
{
console.log('client#close');
connected = false;
setTimeout(connect, 333);
});
client.on('connect', () =>
{
console.log('client#connect');
connected = true;
});
//client.on('data', (data) => console.log('client#data', data));
///*
client.on('readable', () =>
{
const data = client.read();
if (data)
{
console.log('client#readable', data);
}
});
//*/
connect();
setInterval(() =>
{
if (connected)
{
client.write(Buffer.from([0, 1, 2]));
}
}, 333);
function connect()
{
console.log('connecting...');
client.connect(502, '127.0.0.1');
} server.js
const net = require('net');
const server = net.createServer();
server.on('connection', (conn) =>
{
console.log('server#connection');
conn.on('error', (err) => console.error('conn#error', err.message));
conn.on('close', (err) => console.log('conn#close'));
conn.on('readable', () =>
{
const data = conn.read();
if (data)
{
console.log('conn#readable', data);
conn.write(data);
}
});
});
server.on('error', (err) => console.error(err.message));
server.on('close', () => console.log('server#close'));
server.on('listening', () => console.log('server#listening'));
server.listen(502); |
It might be passing, but it is now failing this test: https://github.com/nodejs/node/blob/main/test/parallel/test-net-connect-paused-connection.js. As to write a test, you should combine your server and client into one file and place it inside |
I tried this, but test I tried
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
'use strict';
require('../common');
const assert = require('assert');
const net = require('net');
const N = 50;
let client_recv_count = 0;
let client_end_count = 0;
let client_readable_count = 0;
let disconnect_count = 0;
const server = net.createServer(function(socket) {
console.error('SERVER: got socket connection');
console.error('SERVER connect, writing');
socket.write('hello\r\n');
socket.on('end', () => {
console.error('SERVER socket end, calling end()');
socket.end();
});
socket.on('close', (had_error) => {
console.log(`SERVER had_error: ${JSON.stringify(had_error)}`);
assert.strictEqual(had_error, false);
});
});
server.listen(0, function() {
console.log('SERVER listening');
const client = net.createConnection(this.address().port);
client.setEncoding('UTF8');
client.on('connect', () => {
console.error('CLIENT connected');
});
client.on("readable", () => {
console.log('CLIENT readable');
client_readable_count++;
client_recv_count += 1;
console.log(`client_recv_count ${client_recv_count}`);
console.error('CLIENT: calling end');
client.end();
});
client.on('end', () => {
console.error('CLIENT end');
client_end_count++;
});
client.on('close', (had_error) => {
console.log('CLIENT disconnect');
assert.strictEqual(had_error, false);
if (disconnect_count++ < N)
client.connect(server.address().port); // reconnect
else
server.close();
});
});
process.on('exit', () => {
assert.strictEqual(disconnect_count, N + 1);
assert.strictEqual(client_recv_count, N + 1);
assert.strictEqual(client_end_count, N + 1);
assert.strictEqual(client_readable_count, N + 1);
}); Result(hangs on last line because client side |
@@ -1033,6 +1033,8 @@ function internalConnect( | |||
self, address, port, addressType, localAddress, localPort, flags) { | |||
// TODO return promise from Socket.prototype.connect which | |||
// wraps _connectReq. | |||
debug('resume'); | |||
self.resume(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only resume if flowing? Or we might lose data when reconnecting a paused stream. @mcollina
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that if the socket is paused or never started, you will force it to start. Causing lost data for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ronag.
Could you just check if any data
event is there (which implies flowing)?
Also please consider to add whatever solution you find here in internalConnectMultiple
, otherwise it will fail with network family autoselection.
FWIW. I think the whole undestroy/reconnect flow for net.Socket is fundamentally flawed and I would personally prefer to just doc deprecate it and emit a warning to anyone that uses it. We should recommend creating a new socket instance instead. |
I agree |
I think you guys are right |
fix readable event not emitted after reconnect
Closes: #25969